feat: don't compress on x-no-compression header #208
Conversation
There was a problem hiding this comment.
function shouldCompress (req, res) {
var type = res.getHeader('Content-Type')
var noCompressionHeader = res.getHeader('x-no-compression')
if (noCompressionHeader) {
debug('%s not compressed', type)
return false
}
if (type === undefined || !compressible(type)) {
debug('%s not compressible', type)
return false
}
return true
}PS: I don't know why but I am not seeing the commit suggestion feature right now, so I have given the code in comment.
bjohansebas
force-pushed
the
x-no-compression
branch
from
7a4b75e to
79cdf57
Compare
|
we shouldn't hardcode tbis behavior. x-no-compression is not a standard http header. the readme already has an example of how to do this within application code |
|
For those looking for the doc referenced above: https://github.com/expressjs/compression?tab=readme-ov-file#filter-1 How do we feel about adding an option for this? While I agree we should not hard code this non-standard behavior, it is quite common for libraries and proxies to implement handling for commonly used non-standard headers. This one feels to me like a reasonable one to support in that way. |
|
hmm, this is where the 'design goals and non-goals' reference might help, but this does not seem to rise to the level of necessitating a first-class option, particularly as it is very easily accomplished in user code |
|
Agreed on the design principals, but I think most of the time those docs should include a "pragmatism clause" to ensure that the guide is not used as a hammer. I agree we could make a better decision if we had clarity, but this options specifically feels like a trade-off since we already take many options for how this package handles compression variations. This seems like small addition following the precedent. |
|
Most of the options are options for 100% agree on the pragmatism. While this would be a small addition, it’s important to ensure that changes are driven by a clear and concrete problem. In this case, it seems like a solution in search of a problem, and I would suggest that the problem may not exist. If we search public code, we see there are less than 100 instances of this header appearing in source code, which indicates very little real world usage. That said, I may be missing something that you have identified. Could you please help me better understand the specific issue this addition is intended to address? |
|
Yeah, a clear description of the problem and where it manifests would be awesome. I am hesitant to add the option with results like that, and I know personally I have never seen this header in use in my work. |
|
Yes, I can actually use the filter instead of adding it directly to the package, unless other people also need it, but for now, I don’t think so. |
We should observe when
X-No-Compressionis sent in the request.